Address a performance regression from #4230 by deferring `new URL` computations. #4344

chrisbobbe · 2020-12-16T20:34:35Z

Following some profiling with Chrome DevTools, and discussion on CZO, keep going in the direction of deferring new URL computations, since the computations done in api.registerForEvents have made the initial fetch take quite a lot longer after #4230 than before, and URL computations were observed to be a big part of that in profiling.

The above measurements are taken with release builds on my 4+ year-old iPhone 6s running iOS 13.7, on CZO, at notable commits (see the chart's key):

998947d (narrow [nfc]: Unexport pmNarrowFromEmails!) — Before Crunchify shell for avatar URLs, handle user_avatar_url_field_optional client capability #4230
c642677 (registerForEvents: Start sending user_avatar_url_field_optional.) — Tip of Crunchify shell for avatar URLs, handle user_avatar_url_field_optional client capability #4230
Then each commit in this series. Note that they're based on Crunchify shell for avatar URLs, handle user_avatar_url_field_optional client capability #4230; I haven't rebased them atop master, which contains more recent commits.

The logging code is the following (also pasted in discussion):

diff --git src/message/fetchActions.js src/message/fetchActions.js
index a610c11ea..46cb1e189 100644
--- src/message/fetchActions.js
+++ src/message/fetchActions.js
@@ -4,6 +4,7 @@ import type { Narrow, Dispatch, GetState, GlobalState, Message, Action } from '.
 import type { ApiResponseServerSettings } from '../api/settings/getServerSettings';
index a610c11ea..46cb1e189 100644
--- src/message/fetchActions.js
+++ src/message/fetchActions.js
@@ -4,6 +4,7 @@ import type { Narrow, Dispatch, GetState, GlobalState, Message, Action } from '.
 import type { ApiResponseServerSettings } from '../api/settings/getServerSettings';
 import type { InitialData } from '../api/initialDataTypes';
 import * as api from '../api';
+import * as logging from '../utils/logging';
 import { resetToMainTabs } from '../actions';
 import { isClientError } from '../api/apiErrors';
 import {
@@ -309,6 +310,7 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat
   let initData: InitialData;
   let serverSettings: ApiResponseServerSettings;
 
+  const before = Date.now();
   try {
     [initData, serverSettings] = await Promise.all([
       tryFetch(() =>
@@ -331,6 +333,8 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat
     dispatch(logout());
     return;
   }
+  const after = Date.now();
+  logging.logToProfiling({ durationMs: after - before, ip: '192.168.1.7' });

It seems to confirm our expectation that the biggest difference is made by removing the new URL in FallbackAvatarURL's validateAndConstructInstance. That's 9ec9e20 in this revision.

Also, removing a new URL from fromUserOrBotData (which was used to discern which subclass to use) is helpful, as expected. That's 1d3c38a in this revision.

Removing two new URLs in GravatarURL's validateAndConstructInstance seems definitely helpful; that's 1de50cc in this revision.

The remaining changes in this revision, 720547f and 190eb15...aren't large improvements, it seems. The former might have shortened the time by just a hair; the latter (the tool chose purple, like the before-#4230 measurements, oops) actually ended up with a larger mean than the mean for 1de50cc.

Unfortunately, nothing was able to clearly beat the before-#4230 measurements (the lowest purple line shows its mean), but it's close.

chrisbobbe · 2020-12-16T20:36:11Z

Next, I'll make a similar chart for the Recurse realm. I expect we might see different levels of effectiveness for some commits than we see on CZO because Recurse has EMAIL_ADDRESS_VISIBILITY_EVERYONE set. But Recurse also has lots of users.

chrisbobbe · 2020-12-16T21:16:46Z

OK, here's the same thing but on the Recurse realm instead of CZO:

Looks like 9ec9e20 (the one for FallbackAvatarURL) is less helpful on Recurse. The "fallback" case kicks in when .avatar_url is missing; and, while the contract is that (if the client sets user_avatar_url_field_optional) the server can omit it at its sole discretion, we happen to know (I think it's still true) that the current implementation omits it for long-term idle users. It seems likely that there are more long-term idle users on CZO than Recurse, which might be a reason we see this difference in effectiveness.

Looks like 1de50cc is a lot less helpful (if it's helpful at all) on Recurse. I expect that's because Recurse has EMAIL_ADDRESS_VISIBILITY_EVERYONE set, while CZO doesn't. 1de50cc improves the case where the server sends the full Gravatar URL string, which it does if EMAIL_ADDRESS_VISIBILITY_EVERYONE is not set.

Looks like 190eb15 is a lot more helpful on Recurse than on CZO. That optimization affects uploaded avatar URLs that are absolute URLs not on the realm. Recurse sends those absolute URLs (it uses the s3 backend), while CZO does not; that's probably why.

Looks like, still, nothing's quite as fast as it was before #4230, hmm.

chrisbobbe · 2020-12-16T21:24:19Z

Looks like, still, nothing's quite as fast as it was before #4230, hmm.

I think we do expect the initial load to speed up dramatically once we start using long-lived event queues (#3916), in any case. That might be a long way off, though.

gnprice

Thanks for these changes and the detailed timing information!

These all look good to me except the last one, 190eb15, for uploaded avatars -- comments on that one below. I'll go ahead and merge all the others, and then I'll see about trying to write an improved version of that one.

I do think we should be able to continue to speed this up so that the initial load is faster than before #4230, rather than slower. The purpose of the user_avatar_url_field_optional feature was all about speeding it up, after all. So for the next steps, I'm curious what a profile shows about where the time is going after these changes. We can continue that discussion in the chat thread.

gnprice · 2020-12-16T21:48:49Z

src/utils/avatar.js

+      absoluteOrRelativeUrl.startsWith('/')
+        ? new URL(absoluteOrRelativeUrl, realm)
+        : absoluteOrRelativeUrl,


A couple of thoughts on this one:

This logic is completely correct that if the URL string starts with a /, it's definitely a relative-URL string (provided it's any kind of valid URL string, and I guess has no fragment). With just a bit more logic, I think we can actually be more specific: we can make it definitely a "path-absolute-URL string", and then we can say ${realm.origin}${absoluteOrRelativeURL} and skip the new URL here too.

If the string doesn't start with a slash, it isn't necessarily absolute; it could be one of a few other flavors of relative-URL strings. I think probably a well-behaved server will never want to give us one of those... but I'm not sure of that. So in the spirit of the crunchy shell, I'd like to validate it just a little more to be sure it's absolute, and if it's neither absolute nor the flavor of relative that we expect, then give an error.

How about I try developing a commit for this uploaded-avatar case? I have the URL Standard open, and I think I can make something short and simple that reliably does the small part of it we need here.

How about I try developing a commit for this uploaded-avatar case? I have the URL Standard open, and I think I can make something short and simple that reliably does the small part of it we need here.

Cool, sounds good, thanks! 🙂

…tor. Specifically, in its static `validateAndConstructInstance` method. In working on the recently merged zulip#4230, we found that calling `new URL` with ordinary input is quite expensive, and so we arranged to not call it when rehydrating. We left the call in `validateAndConstructInstance`, though, thinking we couldn't possibly remove it and still validate the raw server data properly at the edge. When we did some performance benchmarking after zulip#4230 was merged [1], though, we found that `api.registerForEvents` started to take a lot longer after zulip#4230 than before it. Profiling [2] suggested that we could help speed things up if we could just stop calling `new URL` in `validateAndConstructInstance`. Looking closer, thankfully, it's possible and reasonably easy. We would normally avoid string concatenation for URLs entirely, but this is a case where we can do it in a fairly principled way (see the implementation comment in the diff). [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1080919 [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1081240 [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1081253

Just as in the previous commit, we're reducing the computation it takes to process avatar URLs at the edge, just as they're received in bulk (of potentially thousands, on a large realm like CZO) from the server. See Greg's comment for profiling data suggesting this change is indicated [1]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1081272

Just as in previous commits, we're reducing the computation it takes to process avatar URLs at the edge, just as they're received in bulk (potentially thousands, on a large realm like CZO) from the server.

This follows up on zulip#4344 to address part of the remaining performance regression from zulip#4230. We still construct URL objects in the `get` methods, lazily and memoized; but never up front when simply taking in all the initial data from the server, or the rehydrated data we've stored. This should save a further slice of time in the initial load.

This follows up on #4344 to address part of the remaining performance regression from #4230. We still construct URL objects in the `get` methods, lazily and memoized; but never up front when simply taking in all the initial data from the server, or the rehydrated data we've stored. This should save a further slice of time in the initial load.

chrisbobbe requested a review from gnprice December 16, 2020 20:34

gnprice reviewed Dec 16, 2020

View reviewed changes

chrisbobbe added 4 commits December 16, 2020 13:54

avatar: Remove a few URL computations for Gravatar URLs.

78ac0a2

Just as in previous commits, we're reducing the computation it takes to process avatar URLs at the edge, just as they're received in bulk (potentially thousands, on a large realm like CZO) from the server.

avatar: Remove URL computation in GravatarURL's pseudo-constructor.

1f0e42c

Just as in previous commits, we're reducing the computation it takes to process avatar URLs at the edge, just as they're received in bulk (potentially thousands, on a large realm like CZO) from the server.

gnprice force-pushed the temp-perf-avatar branch from 0798f96 to 1f0e42c Compare December 16, 2020 21:57

gnprice merged commit 1f0e42c into zulip:master Dec 16, 2020

gnprice mentioned this pull request Dec 17, 2020

avatar: Remove remaining URL construction for initial load. #4345

Merged

gnprice mentioned this pull request Dec 18, 2020

Finish replacing ad-hoc heuristic URL string processing #4146

Open

chrisbobbe mentioned this pull request Dec 18, 2020

Inline remotedev-serialize, stop using jsan. #4348

Merged

chrisbobbe mentioned this pull request Oct 13, 2021

people: Fix small avatar URL generation zulip/zulip#19962

Merged

chrisbobbe deleted the temp-perf-avatar branch November 4, 2021 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address a performance regression from #4230 by deferring `new URL` computations. #4344

Address a performance regression from #4230 by deferring `new URL` computations. #4344

chrisbobbe commented Dec 16, 2020 •

edited

Loading

chrisbobbe commented Dec 16, 2020 •

edited

Loading

chrisbobbe commented Dec 16, 2020 •

edited

Loading

chrisbobbe commented Dec 16, 2020

gnprice left a comment

gnprice Dec 16, 2020

chrisbobbe Dec 16, 2020

Address a performance regression from #4230 by deferring new URL computations. #4344

Address a performance regression from #4230 by deferring new URL computations. #4344

Conversation

chrisbobbe commented Dec 16, 2020 • edited Loading

chrisbobbe commented Dec 16, 2020 • edited Loading

chrisbobbe commented Dec 16, 2020 • edited Loading

chrisbobbe commented Dec 16, 2020

gnprice left a comment

Choose a reason for hiding this comment

gnprice Dec 16, 2020

Choose a reason for hiding this comment

chrisbobbe Dec 16, 2020

Choose a reason for hiding this comment

Address a performance regression from #4230 by deferring `new URL` computations. #4344

Address a performance regression from #4230 by deferring `new URL` computations. #4344

chrisbobbe commented Dec 16, 2020 •

edited

Loading

chrisbobbe commented Dec 16, 2020 •

edited

Loading

chrisbobbe commented Dec 16, 2020 •

edited

Loading